Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide an explicit ComObject<T> type that represents a heap-allocated COM object #3043

Merged
merged 13 commits into from
May 16, 2024

Conversation

sivadeilra
Copy link
Collaborator

@sivadeilra sivadeilra commented May 15, 2024

This adds a new type, ComObject<T>, which represents a type that was annotated with #[implement] and which has been placed in the heap. ComObject is conceptually similar to the ATL/MFC CComObject type.

The ComObject type and the #[implement] macro work together. They provide efficient (zero-cost) ways to cast to the interfaces that T implements. There are no runtime checks (and no addref/release), since we know at compile time that T implements those interfaces.

ComObject is a counted reference to the heap allocation that contains T. Its Clone implementation increments the reference count and its Drop implementation decrements the reference count.

Various other improvements;

  • The #[implement] and #[interface] macros can now be used in crates that are #![no_std]. The windows_core crate is not yet #![no_std], but I did convert a lot of std::foo to core::foo. I don't plan on getting all the way there in this PR, but this helps.
  • Added #[inline(always)] to many functions that are on important call paths. This is partly for performance but mainly for improving the debugging experience for stepping through interface dispatch. Right now, if you step into a call to a COM interface, you spend a lot of time stepping through 1-line functions, long before getting to the actual v-call.
  • Added unit tests for all new features in this PR.
  • ComObject has delegating implementations of many core Rust traits, such as Default, Hash, PartialEq, etc.

@sivadeilra sivadeilra marked this pull request as ready for review May 15, 2024 19:33
crates/libs/core/src/com_object.rs Outdated Show resolved Hide resolved
crates/libs/core/src/com_object.rs Show resolved Hide resolved
crates/libs/core/src/com_object.rs Outdated Show resolved Hide resolved
crates/libs/core/src/com_object.rs Outdated Show resolved Hide resolved
crates/libs/core/src/com_object.rs Show resolved Hide resolved
crates/libs/core/src/com_object.rs Outdated Show resolved Hide resolved
crates/libs/core/src/imp/mod.rs Outdated Show resolved Hide resolved
crates/libs/core/src/interface.rs Show resolved Hide resolved
crates/libs/core/src/com_object.rs Outdated Show resolved Hide resolved
dpaoliello
dpaoliello previously approved these changes May 15, 2024

/// Gets an owned (counted) reference to an interface that is implemented by this [`ComObject`].
#[inline(always)]
pub fn to_interface<I: Interface>(&self) -> I
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How useful is this in practice? Seems like it wouldn't be much harder just writing as_interface().to_owned().

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a convenience. I think we'll need to use this stuff a bunch to see what's the most useful.

/// For those situations where using the [`Deref`] impl is inconvenient, you can use
/// this method to explicitly get a reference to the contents.
#[inline(always)]
pub fn get(&self) -> &T {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking a step back, how useful is ComObject outside the context of object construction? Since get_mut necessarily can't be used without exclusive ownership, it seems as if ComObject would only ever be used for object construction. If that is the case, I'm wondering whether anything can be simplified about this design. It is not overly complicated as it is, just something to consider.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can of course still use get to retrieve a synchronization primitive like RwLock stored within the implementation and go from there. So this could well be used during the life cycle of the object. Is that how you see it being used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's useful outside of just construction. A common pattern in DWriteCore is that we create a bunch of COM objects, and hand out interfaces to the client process, but we also need to keep strongly-typed references to the contents. Right now I'm doing that with the equivalent that is in the old com crate, and having ComObject in windows_core gets me that much closer to converting DWriteCore to use the Windows crates.

get_mut() itself is mainly useful during construction. You create a ComObject, then get access to its guts while you're still the only reference holder. You could get the same effect with RefCell, but since we already have the reference count in ComObject, why not use it? It's basically free.


/// Gets a reference to the shared object's heap box.
#[inline(always)]
pub fn get_box(&self) -> &T::Outer {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an implementation detail - does it need to be public?


/// Returns `true` if this reference is the only reference to the `ComObject`.
#[inline(always)]
pub fn is_exclusive_reference(&self) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an implementation detail - does it need to be public?

kennykerr
kennykerr previously approved these changes May 16, 2024
Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks!

@kennykerr kennykerr merged commit c702f7b into microsoft:master May 16, 2024
89 checks passed
@sivadeilra sivadeilra deleted the user/ardavis/com-object branch May 23, 2024 23:06
mati865 pushed a commit to mati865/windows-rs that referenced this pull request Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants